Skip to content

fix(controllers): switch the PDB from maxUnavailable to minAvailable - #351

Merged
Timofei Larkin (lllamnyp) merged 2 commits into
cozystack:mainfrom
kjvalencik:kj/pdb-min-available
Aug 3, 2026
Merged

fix(controllers): switch the PDB from maxUnavailable to minAvailable#351
Timofei Larkin (lllamnyp) merged 2 commits into
cozystack:mainfrom
kjvalencik:kj/pdb-min-available

Conversation

@kjvalencik

@kjvalencik K.J. Valencik (kjvalencik) commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Production impact

During a node rotation (Karpenter), a healthy 5-voter cluster was drained to 2 voters — below its steady-state quorum of 3 — for ~36 minutes. The budget was maxUnavailable: 2, which should have prevented this, but it re-bases under churn: allowed = currentHealthy - (expectedCount - maxUnavailable), where expectedCount derives from the /scale subresources of the currently matching pods. Each removed member shrank expectedCount and refilled the budget mid-drain (evicted-but-terminating voters also briefly kept counting as healthy). No single eviction violated the budget, yet the cluster ended up at 2-of-2.

The window stayed open that long because recovery didn't converge on its own: a replacement pod crashlooped, having been created with a static config that expected a number of voters that had since shrunk.

Why minAvailable, and why the target anchors it

An integer minAvailable has no expectedCount term — allowed = currentHealthy - minAvailable — so the floor cannot re-base as membership shrinks. The floor is the quorum (n/2 + 1) of max(live voters, status.observed.replicas):

  • Steady state: identical disruptions to the old budget (n - (n-1)/2 = n/2 + 1).
  • Unplanned churn: the latched target holds the floor at steady-state quorum while the operator refills membership. This would have stopped the incident at the first over-budget eviction.
  • Intentional scale-down: the live count dominates, so the floor tracks the live quorum down as members are removed (via MemberRemove, not the eviction API) — a 5→3 shrink never wedges drains.
  • Bootstrap / scale-up: the target dominates; voter evictions block until the cluster reaches size. Learners are outside the selector and unaffected.

Existing PDBs are migrated in place: the reconciler clears maxUnavailable in the same patch that sets minAvailable (a PDB with both is invalid). No API change; selector, delete-at-zero-voters, additionalMetadata, and the /scale contract are untouched.

Future direction

This is deliberately the least impactful fix: no new API, steady-state semantics unchanged. What I'd actually like is to allow only a single voter to be replaced at a time regardless of cluster size (minAvailable = n - 1) — rotations replace nodes serially anyway, and quorum is the emergency floor, not a comfortable operating point. That needs configuration and more complex code, so it's left to a future improvement. Feedback on that design is welcome.

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes

    • Improved PodDisruptionBudget handling to preserve quorum during disruptions, scaling, and membership changes.
    • Migrated existing budgets from maximum-unavailable rules to minimum-available quorum protection.
    • PodDisruptionBudgets are removed when a cluster has no voting members.
    • Stabilized disruption protection during voter churn and cluster scale transitions.
  • Documentation

    • Updated user guidance and API descriptions to explain quorum-based disruption protection, scaling behavior, and node-draining expectations.
    • Clarified migration messaging for legacy PodDisruptionBudget configurations.

The generated PodDisruptionBudget set maxUnavailable = (voters-1)/2 over
the role=voter pods. That form re-bases under churn: the disruption
controller computes allowed = currentHealthy - (expectedCount -
maxUnavailable), and expectedCount is derived from the EtcdMember /scale
subresources of the currently-matching pods. During a node rotation each
removed member shrank expectedCount, refilling the budget mid-drain — a
3-of-5 production cluster was legally evicted down to 2-of-2 quorum for
~36 minutes (2026-07-30 incident).

An integer minAvailable has no expectedCount term (allowed =
currentHealthy - minAvailable), so the floor cannot move as membership
shrinks. The floor is the quorum (n/2+1) of max(live voters,
status.observed.replicas):

  - Steady state: identical disruptions to the old budget, since
    n - (n-1)/2 = n/2 + 1.
  - Unplanned churn: the latched target holds the floor at steady-state
    quorum while the operator refills membership; this would have
    clamped the incident.
  - Intentional scale-down: the live count dominates and steps the
    floor down as members are removed via MemberRemove (not the
    eviction API), so a 5->3 shrink never wedges drains.
  - Bootstrap/scale-up: the target dominates and voter evictions block
    until the cluster reaches size; learners are outside the selector
    and unaffected.

The update path now also migrates PDBs left by previous versions:
maxUnavailable is cleared in the same patch that sets minAvailable
(a PDB with both fields is invalid). Delete-at-zero-voters, the
role=voter selector, and additionalMetadata merging are unchanged;
no API surface is added.

The EtcdMember /scale contract also stays, but its comments are
corrected: they claimed the PDB controller requires /scale and goes
SyncFailed without it, which is only true of maxUnavailable and
percentage minAvailable budgets — an integer minAvailable takes
expectedCount = len(selected pods) and never resolves scale. The
subresource is kept for any user-created budget over member Pods that
does resolve scale.

Signed-off-by: K.J. Valencik <kjvalencik@gmail.com>
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@github-actions github-actions Bot added api-change controllers documentation Improvements or additions to documentation labels Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The operator now manages PodDisruptionBudgets with quorum-based MinAvailable, based on live voters and observed target replicas. Tests cover migration and churn. API comments and documentation describe the updated /scale, PDB, and drain behavior.

Changes

PodDisruptionBudget quorum behavior

Layer / File(s) Summary
Scale subresource contract
api/v1alpha2/etcdmember_types.go, charts/etcd-operator/crd-bases/..., controllers/etcdmember_controller.go
Replica and selector documentation describes /scale exposure and scale-resolving disruption budgets.
MinAvailable reconciliation
controllers/etcdcluster_controller.go
PDB reconciliation computes quorum from live voters and observed target replicas, creates MinAvailable budgets, and migrates existing MaxUnavailable budgets.
Behavior validation and guidance
controllers/etcdcluster_controller_test.go, README.md, docs/concepts.md, docs/operations.md, internal/migrate/translate.go, internal/migrate/translate_test.go
Tests cover quorum calculations, migration, churn, and zero-voter deletion. Documentation and migration warnings describe quorum anchoring and drain behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested labels: bugfix

Suggested reviewers: androndo

Sequence Diagram(s)

sequenceDiagram
  participant EtcdClusterController
  participant ClusterStatus
  participant PodDisruptionBudget
  EtcdClusterController->>ClusterStatus: read observed replicas
  EtcdClusterController->>EtcdClusterController: calculate quorum MinAvailable
  EtcdClusterController->>PodDisruptionBudget: create or patch MinAvailable
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change from maxUnavailable to minAvailable in the PodDisruptionBudget.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/concepts.md`:
- Line 387: Update the quorum-floor documentation: in docs/concepts.md:387-387,
replace the claim that voter evictions are entirely blocked below target with
the behavior that voluntary disruptions are prevented only below quorum(max(live
voters, target)), while scale-down follows the larger live-voter anchor; in
README.md:24-24, mention the max(live voters, intended target) anchor or link to
the detailed formula; in docs/operations.md:541-541, remove the claim that all
voter evictions are blocked until membership is whole.
- Around line 399-402: The scale-up safety discussion around “Scale-up (after
promote)” incorrectly permits N=1, where evicting the unlabelled promoted voter
violates quorum. Update the implementation or documented topology to ensure the
incoming voter is PDB-selected before promotion, or explicitly prevent 1→2
scale-ups, and add a regression case covering this scenario; do not rely on the
quorum floor to protect Pods outside the selector.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3ebde05c-b355-4ae6-8e5f-48e83b229e54

📥 Commits

Reviewing files that changed from the base of the PR and between 9c5d896 and d56e6ca.

📒 Files selected for processing (8)
  • README.md
  • api/v1alpha2/etcdmember_types.go
  • charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdmembers.yaml
  • controllers/etcdcluster_controller.go
  • controllers/etcdcluster_controller_test.go
  • controllers/etcdmember_controller.go
  • docs/concepts.md
  • docs/operations.md

Comment thread docs/concepts.md Outdated
Comment thread docs/concepts.md
Two descriptions of the new minAvailable budget were inaccurate.

The migration tool still told users their dropped
spec.podDisruptionBudgetTemplate would be replaced by a PDB with
maxUnavailable=(voters-1)/2, a shape the operator no longer emits.
Describe the emitted budget instead. The kitchen-sink warning
assertion matched only the "spec.podDisruptionBudgetTemplate" prefix,
so it passed against the stale text; it now pins the full sentence.

The docs claimed voter evictions "block entirely" while the cluster is
below target. Allowed disruptions are healthy - (target/2 + 1), so
being below target is not sufficient: at target 5 with 4 healthy
voters one eviction is still allowed, at target 7 with 6 healthy two
are. The claim holds only for a 3-member target, where any shortfall
does reach the floor. Reword concepts.md and operations.md to state
the actual condition — evictions block once healthy voters fall to
floor(target/2)+1 — and add table rows to TestPDBMinAvailable pinning
the anchors behind those numbers. README dropped the max(live, target)
half of the formula, which is the branch that keeps a 5->3 scale-down
from wedging drains; restore it.

Also rename the local min to minAvail in reconcilePDB so it stops
shadowing the builtin nine lines below a use of the builtin max.

No behaviour change: pdbMinAvailable and reconcilePDB are untouched
apart from the rename.

Assisted-By: Claude Opus 5
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@controllers/etcdcluster_controller_test.go`:
- Around line 3188-3190: Update the comment near the healthy-voter disruption
test to narrow the target range from “targets > 3” to “targets >= 5,” or
explicitly state that only some larger targets permit a one-voter shortfall;
keep the quorum and disruption behavior description accurate.

In `@internal/migrate/translate.go`:
- Line 106: Update the migration warning in the translation logic to describe
the operator’s target-based PDB floor: quorum is calculated from max(live
voters, status.observed.replicas), and the PDB is deleted when no voters remain.
Update the matching expected warning in the relevant translate test to reflect
this wording and behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9f612fa5-c539-48f2-bc28-825b90ac1b30

📥 Commits

Reviewing files that changed from the base of the PR and between d56e6ca and 4b3ef1d.

📒 Files selected for processing (7)
  • README.md
  • controllers/etcdcluster_controller.go
  • controllers/etcdcluster_controller_test.go
  • docs/concepts.md
  • docs/operations.md
  • internal/migrate/translate.go
  • internal/migrate/translate_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • docs/operations.md
  • docs/concepts.md
  • README.md
  • controllers/etcdcluster_controller.go

Comment thread controllers/etcdcluster_controller_test.go
Comment thread internal/migrate/translate.go
@lllamnyp
Timofei Larkin (lllamnyp) merged commit a575cdb into cozystack:main Aug 3, 2026
10 checks passed
@lllamnyp

Copy link
Copy Markdown
Member

K.J. Valencik (@kjvalencik) thanks for your work on this, merged.

K.J. Valencik (kjvalencik) added a commit to kjvalencik/etcd-operator that referenced this pull request Aug 3, 2026
The crash-loop self-heal (cozystack#336) excluded memory-medium members on the
theory that the pod-loss self-heal already covers them. That check only
fires when the Pod is gone or replaced (Status.PodUID mismatch); a
memory member whose Pod is alive but whose etcd can never start keeps
the same Pod UID forever and is invisible to it.

The live failure mode: a replacement learner is created with
--initial-cluster baked into its immutable Pod spec; if membership
changes again before the learner's first successful boot, etcd fatals
with "error validating peerURLs ...: member count is unequal" on every
restart. With max-learners=1 the wedged learner also blocks all further
member replacement, so the cluster stays degraded until a human (or a
node rotation that happens to kill the Pod) intervenes.

Drop the medium exclusion. Every other guard stays: non-bootstrap,
etcdContainerStuck (not ready, restart threshold, not OOMKilled, Pod not
terminating) and the quorum gate. Replacing a stuck memory member is
strictly milder than the already-covered PVC case — its data dies with
the Pod anyway.

Related: cozystack#351
Signed-off-by: K.J. Valencik <kjvalencik@gmail.com>
K.J. Valencik (kjvalencik) added a commit to kjvalencik/etcd-operator that referenced this pull request Aug 3, 2026
The crash-loop self-heal (cozystack#336) excluded memory-medium members on the
theory that the pod-loss self-heal already covers them. That check only
fires when the Pod is gone or replaced (Status.PodUID mismatch); a
memory member whose Pod is alive but whose etcd can never start keeps
the same Pod UID forever and is invisible to it.

The live failure mode: a replacement learner is created with
--initial-cluster baked into its immutable Pod spec; if membership
changes again before the learner's first successful boot, etcd fatals
with "error validating peerURLs ...: member count is unequal" on every
restart. With max-learners=1 the wedged learner also blocks all further
member replacement, so the cluster stays degraded until a human (or a
node rotation that happens to kill the Pod) intervenes.

Drop the medium exclusion. Every other guard stays: non-bootstrap,
etcdContainerStuck (not ready, restart threshold, not OOMKilled, Pod not
terminating) and the quorum gate. Replacing a stuck memory member is
strictly milder than the already-covered PVC case — its data dies with
the Pod anyway.

Related: cozystack#351
Signed-off-by: K.J. Valencik <kjvalencik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-change controllers documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants